feat(standards): P2ID/P2IDE notes must carry at least one asset by VAIBHAVJINDAL3012 · Pull Request #2986 · 0xMiden/protocol

VAIBHAVJINDAL3012 · 2026-05-26T20:08:27Z

This PR add the assertion check for the case that there should be atleast one asset in the note when add_assets_to_account function is called.

[BREAKING] Enforced at both layers:

Rust: P2idNote::create / P2ideNote::create return the new NoteError::MissingAsset on empty assets.
MASM: basic_wallet::add_assets_to_account asserts num_assets > 0 (new ERR_BASIC_WALLET_EMPTY_NOTE_ASSETS).

closes #2978

[BREAKING] Enforced at both layers: - Rust: P2idNote::create / P2ideNote::create return the new NoteError::MissingAsset on empty assets. - MASM: basic_wallet::add_assets_to_account asserts num_assets > 0 (new ERR_BASIC_WALLET_EMPTY_NOTE_ASSETS). Adds unit tests for both rejection paths. Kernel-test fixtures that constructed 0-asset P2ID notes are updated: incidental sites pass a dummy FungibleAsset; the dedicated p2id_note_0_assets scenario in TestSetup is dropped (1- and 2-asset coverage remains).

bobbinth · 2026-05-26T21:37:00Z

Question (without having looked at the code): do we want to do anything about a fungible asset with 0 amount? Technically, it is an asset, but conceptually, it is the same as no assets.

VAIBHAVJINDAL3012 · 2026-05-26T21:40:48Z

Question (without having looked at the code): do we want to do anything about a fungible asset with 0 amount? Technically, it is an asset, but conceptually, it is the same as no assets.

Technically we shouldn't allow it but it still doesn't have any attack vector. We can also put checks while transferring asset to account that asset value shouldn't be 0.

bobbinth · 2026-05-26T21:46:17Z

We can also put checks while transferring asset to account that asset value shouldn't be 0.

This may be tricky as we can't check this for all assets in general (especially, at the protocol level) - e.g., "custom" assets would be excluded.

VAIBHAVJINDAL3012 · 2026-05-26T21:47:32Z

This may be tricky as we can't check this for all assets in general (especially, at the protocol level) - e.g., "custom" assets would be excluded.

Which assets have value 0?

bobbinth · 2026-05-26T22:01:26Z

This may be tricky as we can't check this for all assets in general (especially, at the protocol level) - e.g., "custom" assets would be excluded.

Which assets have value 0?

Fungible assets currently. In the future, we'll have "custom" asset with asset-specific structure - so, inferring whether an asset is a "zero" asset by looking at the asset itself, may not be possible in generality.

But we may be able to do the check in a different way: if the root of the asset vault didn't change after the asset was added, we know that adding an asset didn't have an effect. I haven't looked into the code to know how easy/viable it is to perform this check - but it may be not too bad.

PhilippGackstatter · 2026-06-05T10:28:51Z


-/// Return a [`TestSetup`], whose notes contain 0, 1 and 2 assets respectively.
+/// Return a [`TestSetup`], whose notes contain 1 and 2 assets respectively.
 fn setup_test() -> anyhow::Result<TestSetup> {


I think we should retain the 0 assets test case here as the tests that use this setup, like the get_assets test, behave meaningfully different from the non-zero asset use case. So, maybe we can just change the test setup to return P2ANY notes instead of P2ID notes (see create_p2any_note, add_p2any_note).

Got it, I will make this change.

PhilippGackstatter · 2026-06-05T10:34:38Z

 #! Adds all assets from the active note to the native account's vault.
 #!
 #! Inputs:  []
 #! Outputs: []
 @locals(512)
 pub proc add_assets_to_account
    # write assets to local memory starting at offset 0
    # we have allocated ASSET_SIZE * MAX_ASSETS_PER_NOTE number of locals so all assets should fit
    # since the asset memory will be overwritten, we don't have to initialize the locals to zero
    locaddr.0 exec.active_note::get_assets
    # => [num_of_assets]

+    # ensure the note carries at least one asset
+    dup neq.0 assert.err=ERR_BASIC_WALLET_EMPTY_NOTE_ASSETS
+    # => [num_of_assets]
+


I'm not sure if this procedure, at the wallet level, should enforce "at least one asset". I think the procedure should be a no-op if no assets exist. I think we should check this at note level instead. For this, we would need to expose active_note::get_assets_info so we can somewhat cheaply get the number of assets without having to write them to memory (like active_note::get_assets would do).

Separately, at the note level, should we add the check that @bobbinth mentioned (#2986 (comment))?

Yes, I thought over it to checks at note level, but I was making redundant checks on P2ID and P2IDE, then I thought of this approach.

we may be able to do the check in a different way: if the root of the asset vault didn't change after the asset was added, we know that adding an asset didn't have an effect. I haven't looked into the code to know how easy/viable it is to perform this check - but it may be not too bad.
Will try for this check.

Yes, I thought over it to checks at note level, but I was making redundant checks on P2ID and P2IDE, then I thought of this approach.

I think the original motivation was to have this check for P2ID* notes, but with this change, we'd also affect other notes that use this API, which may or may not want to implement this check. I think the add_assets_to_account API is more useful if it doesn't require a non-zero number of assets. If users would want that behavior with the latest changes, they'd have to wrap it in an if-else branch.

So, I'd move these checks to the P2ID* notes. I don't think the duplication is an issue in this case.

Okay, I will implement this in P2ID and P2IDE note.

partylikeits1983

Maybe I am missing something, but why does the MASM procedure add_assets_to_account need to check that that the Asset is not zero? Why would it need to revert if the amount is 0?

I think its valuable to add the checks to the note builder rust logic that an Asset amount cannot be zero when creating a note, but I am not sure if this logic should be in the masm logic

bobbinth · 2026-06-11T03:52:33Z

Maybe I am missing something, but why does the MASM procedure add_assets_to_account need to check that that the Asset is not zero? Why would it need to revert if the amount is 0?

I think its valuable to add the checks to the note builder rust logic that an Asset amount cannot be zero when creating a note, but I am not sure if this logic should be in the masm logic

I think I agree with this. The current check in MASM doesn't buy us much - i.e., if somebody erroneously created a P2ID note w/o any assets, this would just make the note unconsumable - but wouldn't help with preventing creating such a note in the first place.

So, maybe checks on the Rust side is all we need here.

VAIBHAVJINDAL3012 · 2026-06-11T07:29:13Z

Maybe I am missing something, but why does the MASM procedure add_assets_to_account need to check that that the Asset is not zero? Why would it need to revert if the amount is 0?
I think its valuable to add the checks to the note builder rust logic that an Asset amount cannot be zero when creating a note, but I am not sure if this logic should be in the masm logic

I think I agree with this. The current check in MASM doesn't buy us much - i.e., if somebody erroneously created a P2ID note w/o any assets, this would just make the note unconsumable - but wouldn't help with preventing creating such a note in the first place.

So, maybe checks on the Rust side is all we need here.

I was also of the same view. (#2978 (comment)) Adding the masm check doesn't bring any security value. But @PhilippGackstatter was of the view that we should keep rust and masm consistent.

mmagician · 2026-06-11T07:50:41Z

Sorry for being late to the discussion.
I agree with @partylikeits1983 and @bobbinth that the check isn't very useful on the MASM side.

In general, we could imagine some network accounts hardcoding a few note script roots in their allowlist to allow these notes to be executed by anyone. P2ID would potentially be one such note.
But the risk surfaces once fees are turned on: auto-consumption of a note should not cost more in fees than the received asset.
Requiring that there is at least one asset present would be the first step to tackling this, but that still wouldn't say anything about the amount. Consuming a P2ID carrying asset = 1 while fee = 2 still drains the account of funds.

Auto-consumption conditions should be encoded in the account, and run at consumption time, not in the note. So I don't think it's meaningful to encode these in MASM at all, and let's stick to Rust, with a clear note to mention that the guards are purely for sanity checks and are not enforced onchain.

PhilippGackstatter · 2026-06-11T09:07:58Z

I don't think it's meaningful to encode these in MASM at all, and let's stick to Rust, with a clear note to mention that the guards are purely for sanity checks and are not enforced onchain.

Fine from my side 👍 My main goal was consistency between Rust and MASM as we usually do it.

Separately, @VAIBHAVJINDAL3012 what was your original motivation for wanting the "at least one asset" check? My assumption was that it was to prevent "value-less" P2ID notes, which also prompted the further checks. But we can't meaningfully enforce that a P2ID note contains "value", because value is in the eye of the beholder, so to say.

VAIBHAVJINDAL3012 · 2026-06-11T09:14:56Z

Separately, @VAIBHAVJINDAL3012 what was your original motivation for wanting the "at least one asset" check? My assumption was that it was to prevent "value-less" P2ID notes, which also prompted the further checks. But we can't meaningfully enforce that a P2ID note contains "value", because value is in the eye of the beholder, so to say.

So this discussion started in this thread (#2283 (comment)). Yes, motivation was to prevent the creation of P2ID notes with zero asset.

VAIBHAVJINDAL3012 force-pushed the at-least-one-asset branch from c94a5cc to af33c26 Compare May 26, 2026 20:52

VAIBHAVJINDAL3012 force-pushed the at-least-one-asset branch from af33c26 to 35a4f39 Compare May 26, 2026 21:03

Keinberger requested a review from PhilippGackstatter June 4, 2026 11:35

PhilippGackstatter reviewed Jun 5, 2026

View reviewed changes

mmagician requested a review from partylikeits1983 June 8, 2026 16:23

partylikeits1983 reviewed Jun 10, 2026

View reviewed changes

Conversation

VAIBHAVJINDAL3012 commented May 26, 2026

Uh oh!

bobbinth commented May 26, 2026

Uh oh!

VAIBHAVJINDAL3012 commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobbinth commented May 26, 2026

Uh oh!

VAIBHAVJINDAL3012 commented May 26, 2026

Uh oh!

bobbinth commented May 26, 2026

Uh oh!

PhilippGackstatter Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

VAIBHAVJINDAL3012 Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

PhilippGackstatter Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

VAIBHAVJINDAL3012 Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

PhilippGackstatter Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

VAIBHAVJINDAL3012 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

partylikeits1983 left a comment

Choose a reason for hiding this comment

Uh oh!

bobbinth commented Jun 11, 2026

Uh oh!

VAIBHAVJINDAL3012 commented Jun 11, 2026

Uh oh!

mmagician commented Jun 11, 2026

Uh oh!

PhilippGackstatter commented Jun 11, 2026

Uh oh!

VAIBHAVJINDAL3012 commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

VAIBHAVJINDAL3012 commented May 26, 2026 •

edited

Loading